Scene entity map when saving scene #22369
Conversation
alice-i-cecile
added
C-Feature
There was a problem hiding this comment.
The logic looks good. Thanks for the test.
What’s the need to have this functionality? Just curious as someone who is new to scenes. I’m assuming it makes it easier to compare two scenes if you’re changing something in between extractions?
I don’t have much context to scene work to determine whether this is controversial or anything, so that’s a caveat!
| component_filter: SceneFilter, | ||
| resource_filter: SceneFilter, | ||
| original_world: &'w World, | ||
| entity_map: Option<&'w mut EntityHashMap<Entity>>, |
There was a problem hiding this comment.
Nit: Not sure if anyone is a fan of too verbose names, but I wonder if the name world_to_scene_entity_map or world_to_scene_map would be better here. I appreciate how succinct entity_map is though, so I may be in the minority
There was a problem hiding this comment.
entity_map is a private field, so it's naming is less important compared to the public fn. I updated the doc of with_entity_map in 7a49f9d so it's more clear that we expect a world -> scene map.
Also write_to_world doesn't say it's a scene -> world entity map. (maybe because this is this is the only direction we considered before this PR). If we want to change, it's better we change in both places.
I also feel user might be confused, because they need to manually create a revert map if they want to use this new API correctly. But I also don't feel super good (and not sure how) to create more guardrail abstractions to make it less confusing.
Another option is use scene -> world map in with_entity_map, but this means DynamicSceneBuilder need to create the revert map inside itself. Also not sure if this is good.
Co-authored-by: Kevin Chen <[email protected]>
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
3 participants
Objective
Entitys more stable by providing the reverse entity map.Solution
with_entity_maptoDynamicSceneBuilderTesting